x/upgrade: added consensus version tracking (part of ADR-041) #8743

technicallyty · 2021-03-02T03:50:08Z

Description

This PR is part of ADR 041 - In-Place Store Migrations

-Adds a new method to module.Manager to get a VersionMap
-Adds a new store prefix 0x2 to x/upgrade to store a VersionMap
-Adds new functionality to x/upgrade to get a VersionMap from state
-Saves a VersionMap on InitGenesis in app.go

You may notice some things in simapp had to be added - this is to avoid circular dependancy. UpgradeKeeper needs a reference to Module Manager, so after the manager is initialized, we give the upgradekeeper a reference to it, and re-set the value of the upgrade keeper in the manager's module map.

closes: #8514

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

amaury1093

x/upgrade's store wrt saving & retrieving module's ConsensusVersions looks good 👍

I'd like to see a test with applying an upgrade handler, and checking the store before and after the upgrade.

x/upgrade/types/keys.go

x/upgrade/keeper/keeper.go

amaury1093 · 2021-03-02T14:14:33Z

x/upgrade/module.go

@@ -101,7 +101,8 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
 }

 // InitGenesis is ignored, no sense in serializing future upgrades
-func (am AppModule) InitGenesis(_ sdk.Context, _ codec.JSONMarshaler, _ json.RawMessage) []abci.ValidatorUpdate {
+func (am AppModule) InitGenesis(ctx sdk.Context, _ codec.JSONMarshaler, _ json.RawMessage) []abci.ValidatorUpdate {
+	InitChainer(am.keeper, ctx)


@tyler I think i might have been mistaken as to when we should call keeper.SetConsensusVersions. We actually don't need to call it in InitChain, just leave it blank on a fresh chain, as per #8504 (comment), I believe it should be called AFTER the handler has run (so here), to say "ok I've ran the handler (e.g. migration scripts), now I'll update the store with the newest modules' ConsensusVersions".

I think the ultimate E2E test would be with 2 binaries, but since that's hard, let's try to simulate that in a test here:

create a mock module.VersionManager, say with bank set to 1, and everything else set to latest ConsensusVersion.

upgradeKeeper.SetModuleManager(mockVersionMgr). This simulates saving the "old" modules version to state.

add an upgrade handler to keeper.

call ApplyUpgrade on keeper

after ApplyUpgrade, call keeper.GetConsensusVersions, test that the return value's bank key should be 2, not 1.

test added and initchainer removed

If it's not set in IntiChain then how do initial versions get set?? I don't see a good alternative and it's not a safe assumption that all chains start at module version 1

I think we need to add this back

InitGenesis added back

simapp/app.go

aaronc

Looks like a great start here @technicallyty 👍. Would be nice to see some grpc query methods in a follow up PR.

…ng consensus version store in x/upgrade -cleaned up x/upgrade Keeper -handler in apply upgrade now handles errors and setting consensus versions -cleaned up migration map keys -removed init chainer method -simapp now implements GetConsensusVersions to assist with testing

codecov · 2021-03-03T18:51:40Z

Codecov Report

Merging #8743 (5d6e4ab) into master (deaee53) will increase coverage by 0.04%.
The diff coverage is 61.76%.

@@            Coverage Diff             @@
##           master    #8743      +/-   ##
==========================================
+ Coverage   59.23%   59.27%   +0.04%     
==========================================
  Files         571      571              
  Lines       31800    31827      +27     
==========================================
+ Hits        18837    18867      +30     
+ Misses      10761    10757       -4     
- Partials     2202     2203       +1

Impacted Files	Coverage Δ
types/module/module.go	`76.13% <0.00%> (-6.58%)`	⬇️
x/upgrade/types/keys.go	`0.00% <ø> (ø)`
x/upgrade/keeper/keeper.go	`76.72% <90.00%> (+2.49%)`	⬆️
simapp/app.go	`83.33% <100.00%> (+0.08%)`	⬆️
testutil/network/util.go	`71.02% <100.00%> (ø)`
x/feegrant/module.go	`47.72% <0.00%> (+2.27%)`	⬆️
x/slashing/module.go	`59.09% <0.00%> (+2.27%)`	⬆️
x/distribution/module.go	`59.09% <0.00%> (+2.27%)`	⬆️
x/evidence/module.go	`46.51% <0.00%> (+2.32%)`	⬆️
x/bank/module.go	`58.13% <0.00%> (+2.32%)`	⬆️
... and 7 more

amaury1093

Nice. ADR-041 has been updated with a bunch of comments from various people. Could you make sure the variable names etc match? 🙏

Two additional things which would be nice to have in this PR:

a changelog (UpgradeHandler is API-breaking)
update x/upgrade/spec docs. Most importantly the state.md one. A link to ADR-041 from one of the other mds would be good too.

x/upgrade/keeper/keeper.go

x/upgrade/keeper/keeper_test.go

x/upgrade/module.go

x/upgrade/keeper/keeper.go

x/upgrade/module_test.go

removed module_test

…llyty/cosmos-sdk into ty-8514-module_tracking

unnecessary newline removed

amaury1093

getting close! Left some nits

x/upgrade/spec/02_state.md

simapp/app.go

x/upgrade/keeper/keeper.go

simapp/app.go

…ons now returns an error

amaury1093

lgtm, thanks @technicallyty 🎉

pinging @aaronc and @robert-zaremba for a review as you both had ideas on ADR-041.

docs/architecture/adr-041-in-place-store-migrations.md

x/upgrade/keeper/keeper.go

x/upgrade/module.go

x/upgrade/module_test.go

x/upgrade/keeper/keeper.go

Co-authored-by: Amaury <[email protected]>

technicallyty · 2021-03-17T19:00:56Z

Changes should be aligned with @aaronc 's suggestions now. Additionally updated identifiers to be more go-like and changed the store get/set methods for VersionMap to align with the return type per @robert-zaremba 's suggestion.

x/upgrade/keeper/keeper.go

aaronc · 2021-03-18T21:27:53Z

x/upgrade/keeper/keeper.go

+	if len(vm) > 0 {
+		store := ctx.KVStore(k.storeKey)
+		versionStore := prefix.NewStore(store, []byte{types.VersionMapByte})
+		for modName, ver := range vm {
+			nameBytes := []byte(modName)
+			verBytes := make([]byte, 8)
+			binary.BigEndian.PutUint64(verBytes, ver)
+			versionStore.Set(nameBytes, verBytes)
+		}
+	}


Why do these not happen in InitGenesis? I thought we agreed upon that.

I do that here

this seemed like correct spot based on the suggestions, did you have somewhere else in mind?

Maybe you're right. I guess I was thinking it could be in InitGenesis but maybe it doesn't matter

aaronc · 2021-03-18T21:29:07Z

Mostly looks good now just one question and a couple suggested renamings.

aaronc

ACK. But I would like to rename those functions -> Get/SetModuleVersions

x/upgrade/keeper/keeper_test.go

…llyty/cosmos-sdk into ty-8514-module_tracking

aaronc · 2021-03-19T21:36:50Z

Some tests are failing here @technicallyty

orijbot · 2021-03-19T22:01:41Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=5bd93bfe7bffef8d8012c97105c2df16c1fecdaf

…#8743) * -added consensus version tracking to x/upgrade * -added interface to module manager -added e2e test for migrations using consensus version store in x/upgrade -cleaned up x/upgrade Keeper -handler in apply upgrade now handles errors and setting consensus versions -cleaned up migration map keys -removed init chainer method -simapp now implements GetConsensusVersions to assist with testing * Changed MigrationMap identifier to VersionMap removed module_test * updated docs * forgot this * added line to changelog for this PR * Change set consensus version function to match adr 041 spec * add documentation * remove newline from changelog unnecessary newline removed * updated example in simapp for RunMigrations, SetCurrentConsensusVersions now returns an error * switch TestMigrations to use Require instead of t.Fatal * Update CHANGELOG.md Co-authored-by: Aaron Craelius <[email protected]> * docs for SetVersionManager * -init genesis method added -removed panics/fails from setting consensus versions * update identifiers to be more go-like * update docs and UpgradeHandler fnc sig * Upgrade Keeper now takes a VersionMap instead of a VersionManager interface * upgrade keeper transition to Version Map * cleanup, added versionmap return to RunMigrations * quick fix * Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Amaury <[email protected]> * remove support for versionmap field on upgrade keeper * cleanup * rename get/set version map keeper functions * update adr doc to match name changes * remove redudant line Co-authored-by: technicallyty <[email protected]> Co-authored-by: Aaron Craelius <[email protected]> Co-authored-by: Amaury <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

-added consensus version tracking to x/upgrade

e900fb1

technicallyty marked this pull request as ready for review March 2, 2021 04:43

technicallyty requested review from aaronc, alessio and alexanderbez as code owners March 2, 2021 04:43

amaury1093 reviewed Mar 2, 2021

View reviewed changes

amaury1093 added the C:x/upgrade label Mar 2, 2021

aaronc reviewed Mar 2, 2021

View reviewed changes

technicallyty and others added 2 commits March 3, 2021 10:58

Merge branch 'master' into ty-8514-module_tracking

b640766

Merge branch 'master' into ty-8514-module_tracking

8c7b105

amaury1093 reviewed Mar 4, 2021

View reviewed changes

technicallyty and others added 9 commits March 4, 2021 09:09

Changed MigrationMap identifier to VersionMap

5c1ae59

removed module_test

updated docs

4c48f35

forgot this

6db21e3

added line to changelog for this PR

c9b435a

Change set consensus version function to match adr 041 spec

1c5b132

add documentation

304abfa

Merge branch 'master' into ty-8514-module_tracking

d06bbbd

Merge branch 'ty-8514-module_tracking' of https://github.com/technica…

5c75ead

…llyty/cosmos-sdk into ty-8514-module_tracking

remove newline from changelog

e53423e

unnecessary newline removed

technicallyty requested a review from amaury1093 March 8, 2021 17:30

amaury1093 reviewed Mar 8, 2021

View reviewed changes

x/upgrade/spec/02_state.md Outdated Show resolved Hide resolved

simapp/app.go Outdated Show resolved Hide resolved

x/upgrade/keeper/keeper.go Outdated Show resolved Hide resolved

simapp/app.go Outdated Show resolved Hide resolved

amaury1093 reviewed Mar 8, 2021

View reviewed changes

simapp/app.go Outdated Show resolved Hide resolved

updated example in simapp for RunMigrations, SetCurrentConsensusVersi…

c36086f

…ons now returns an error

technicallyty requested a review from amaury1093 March 8, 2021 19:43

switch TestMigrations to use Require instead of t.Fatal

758c67d

amaury1093 approved these changes Mar 9, 2021

View reviewed changes

amaury1093 requested a review from robert-zaremba March 9, 2021 09:49

technicallyty added 2 commits March 16, 2021 11:41

cleanup, added versionmap return to RunMigrations

799b646

quick fix

0f9447c

clevinson requested a review from robert-zaremba March 16, 2021 21:29

amaury1093 reviewed Mar 17, 2021

View reviewed changes

technicallyty and others added 5 commits March 17, 2021 09:09

Merge branch 'master' into ty-8514-module_tracking

a157c48

Update docs/architecture/adr-041-in-place-store-migrations.md

b839a59

Co-authored-by: Amaury <[email protected]>

remove support for versionmap field on upgrade keeper

75f8ae5

cleanup

22ab55e

merge adr change

47ce533

aaronc reviewed Mar 18, 2021

View reviewed changes

aaronc approved these changes Mar 18, 2021

View reviewed changes

technicallyty and others added 3 commits March 18, 2021 15:20

rename get/set version map keeper functions

7f5b6aa

update adr doc to match name changes

8547426

Merge branch 'master' into ty-8514-module_tracking

1504a2d

amaury1093 approved these changes Mar 19, 2021

View reviewed changes

x/upgrade/keeper/keeper_test.go Outdated Show resolved Hide resolved

amaury1093 mentioned this pull request Mar 19, 2021

Add x/upgrade gRPC query for module VersionMap #8941

Closed

4 tasks

technicallyty added 2 commits March 19, 2021 06:59

remove redudant line

c3072ab

Merge branch 'ty-8514-module_tracking' of https://github.com/technica…

1f82fff

…llyty/cosmos-sdk into ty-8514-module_tracking

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 19, 2021

technicallyty and others added 4 commits March 19, 2021 07:03

Merge branch 'master' into ty-8514-module_tracking

cf1343f

Merge branch 'master' into ty-8514-module_tracking

3f6bea8

Merge branch 'master' into ty-8514-module_tracking

d987555

Merge branch 'master' into ty-8514-module_tracking

5d6e4ab

mergify bot merged commit 5bd93bf into cosmos:master Mar 19, 2021

0xHansLee mentioned this pull request Jun 23, 2022

feat: upgrade cosmos-sdk v0.45.4 medibloc/panacea-core#356

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/upgrade: added consensus version tracking (part of ADR-041) #8743

x/upgrade: added consensus version tracking (part of ADR-041) #8743

technicallyty commented Mar 2, 2021 •

edited

Loading

amaury1093 left a comment

amaury1093 Mar 2, 2021 •

edited

Loading

technicallyty Mar 3, 2021

aaronc Mar 10, 2021

aaronc Mar 10, 2021

technicallyty Mar 11, 2021

aaronc left a comment

codecov bot commented Mar 3, 2021 •

edited

Loading

amaury1093 left a comment

amaury1093 left a comment

amaury1093 left a comment

technicallyty commented Mar 17, 2021

aaronc Mar 18, 2021

technicallyty Mar 18, 2021

technicallyty Mar 18, 2021

aaronc Mar 18, 2021

aaronc commented Mar 18, 2021

aaronc left a comment

aaronc commented Mar 19, 2021

orijbot commented Mar 19, 2021

x/upgrade: added consensus version tracking (part of ADR-041) #8743

x/upgrade: added consensus version tracking (part of ADR-041) #8743

Conversation

technicallyty commented Mar 2, 2021 • edited Loading

Description

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2021 • edited Loading

Codecov Report

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

technicallyty commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Mar 18, 2021

aaronc left a comment

Choose a reason for hiding this comment

aaronc commented Mar 19, 2021

orijbot commented Mar 19, 2021

technicallyty commented Mar 2, 2021 •

edited

Loading

amaury1093 Mar 2, 2021 •

edited

Loading

codecov bot commented Mar 3, 2021 •

edited

Loading